Add reset and barrierAction to CyclicBarrier. #3055

HSAR · 2023-05-17T13:13:08Z

Both reset() and getNumberWaiting() are methods that exist on java.util.concurrent.CyclicBarrier and function in the same way.

This PR comes from discussion on issue #3054.

Both reset() and getNumberWaiting() are methods that exist on java.util.concurrent.CyclicBarrier and function in the same way.

HSAR · 2023-05-17T13:14:29Z

I'm opening this PR in the hope of starting a conversation about the implementation and tests, both of which I'm aware could do with some improvement.

arrow-libs/fx/arrow-fx-coroutines/src/commonMain/kotlin/arrow/fx/coroutines/CyclicBarrier.kt

This is also from java.util.concurrent.CyclicBarrier and executes when the barrier opens due to its capacity being reached.

nomisRev · 2023-05-25T14:51:47Z

@HSAR thank you so much for the PR! I need to make some time to properly look at these changes, sorry it's been taking so long but it's not a PR I can quickly review 😅

On first sight it looks pretty good, but I need to dig around a bit and perhaps write some tests for myself. Concurrency is hard 😅

HSAR · 2023-05-25T14:54:13Z

No problem. Keep me posted if/when you have questions or comments!

nomisRev · 2023-07-03T11:35:57Z

Hey @HSAR,

There are some tests hanging on CI, and also for me locally. Not sure if you have time to look into it, but I am also taking a look at it whilst reviewing the code.

HSAR · 2023-07-03T12:03:08Z

Hi @nomisRev - happy to take a look. I had thought that everything was passing when I left it last time, but I am very unfamiliar with this test framework and it is possible that I wasn't running it correctly.

nomisRev · 2023-07-03T12:12:56Z

@HSAR I think we lost some history due to force-push, because I remember it passing at some time. You should be able to run the tests from IDEA. I think you can ignore checkAll for these tests too if you want. Or write them in Kotlin-test if you're more familiar with that. I'd prefer to have better tests, having some style difference in them is low-tech debt in favour of better tests IMO.

I can always help you refactor them to Kotest if really need to.

HSAR · 2023-07-03T12:37:30Z

I expected to be able to run the tests from IDEA, but that doesn't seem to be the case (at least, not by default), which rather puzzled me. I'm personally only familiar with JUnit, but I am perfectly happy with any test framework if I can get it to run properly in my IDE. Are there extra steps I need to follow?

Regarding the review, I restructured the code more than once to try and avoid problems with the reset action (which has turned out to be really rather tricky). This one works in many of scenarios but I don't think it is watertight - I am worried for instance about StateMachine::startNextEpoch which seems like it is not nearly atomic enough.

nomisRev · 2023-07-04T12:24:53Z

Hey @HSAR,

I am taking a stab at it from scratch, to see how far I can get it. That's easier for me to get my head around it 😅 I'll keep you posted today or tomorrow, so we can discuss code in further detail.

To run Kotest you should install the IDEA Kotest plugin, but it sometimes doesn't behave perfectly. I can for example not run individual tests, only entire test files which is kind-of unfortunate. I've contemplated refactoring to JUnit Kotlin-test to leverage things like WASM, but that seems like a too big of an effort for now.

HSAR · 2023-07-04T12:58:55Z

I might hold off trying to fix the tests, as it seems entirely possible we might junk the current implementation anyway. Happy to chat code if it would help.

nomisRev · 2023-07-05T10:17:56Z

Hey @HSAR,

I played a little bit around with the implementation and came up with #3076 What do you think? Not sure what the best approach is, because I don't want to throw out your effort. I think you should get credit for leading this in the git history.

…to sv-cyclicbarrier-reset

HSAR · 2023-07-05T13:38:04Z

I like the code inside ::reset and the use of a specific CyclicBarrierCancellationException, both are a lot tidier. A few things that do come to mind, though:

The epoch doesn't advance when a reset happens. That is of course part of the internal logic and state of CyclicBarrier so is acceptable from an external API point of view, I just thought it might be useful to point out. The two State possibilities should, I presume, protect from any thread-unsafe-ness.
Keeping the barrierAction feature would be quite desirable: my implementation used it as a neat way of implementing the reset but that was not the main reason it existed, which was both because it was a useful feature and because it was part of the original java.util.concurrent CyclicBarrer.

As for the git history, the code is yours and so should be the git blame ;)
Jokes aside I won't lie, it would be nice to be recognised for contribution. Perhaps if you agree with my second point then I can add the barrierAction and thus obtain a little credit if/when all is merged.

HSAR · 2023-07-05T13:42:13Z

PS. Unused imports at the top of the file ;)

nomisRev · 2023-07-06T13:45:06Z

Hey @HSAR,
I am closing my other PR. I merged the changes and tried to keep as much as your changes as possible to give you proper credit for driving this effort 🙏 It was not easy 😅

Sorry the entire process was so extraordinarily long.. Please do a thorough review of all my work, and I think with a review from @serras or @franciscodr we should be good to merg this work 🙌

HSAR · 2023-07-06T13:54:02Z

Hey @nomisRev,

I was a bit confused because I had pushed a different, more-conservative merge not too long ago. LGTM apart from one comment which I've left inline.

I'm glad to have contributed! Thanks for your time looking into the issue and understanding the problems.

arrow-libs/fx/arrow-fx-coroutines/src/commonMain/kotlin/arrow/fx/coroutines/CyclicBarrier.kt

serras

Nice job!

arrow-libs/fx/arrow-fx-coroutines/src/commonMain/kotlin/arrow/fx/coroutines/CyclicBarrier.kt

...w-libs/fx/arrow-fx-coroutines/src/commonTest/kotlin/arrow/fx/coroutines/CyclicBarrierSpec.kt

Co-authored-by: Alejandro Serrano <trupill@gmail.com>

nomisRev · 2023-07-12T11:46:47Z

Thank you for the awesome work @HSAR! Congrats on your first contribution 🙌 Very impressive work! 👏

HSAR · 2023-07-12T12:01:19Z

Happy to have my contribution accepted. Thanks for your part in this too!

[![Mend Renovate](https://app.renovatebot.com/images/banner.svg)](https://renovatebot.com) This PR contains the following updates: | Package | Change | Age | Adoption | Passing | Confidence | |---|---|---|---|---|---| | [io.arrow-kt:arrow-optics-ksp-plugin](https://togithub.com/arrow-kt/arrow) | `1.1.5` -> `1.2.0` | [![age](https://badges.renovateapi.com/packages/maven/io.arrow-kt:arrow-optics-ksp-plugin/1.2.0/age-slim)](https://docs.renovatebot.com/merge-confidence/) | [![adoption](https://badges.renovateapi.com/packages/maven/io.arrow-kt:arrow-optics-ksp-plugin/1.2.0/adoption-slim)](https://docs.renovatebot.com/merge-confidence/) | [![passing](https://badges.renovateapi.com/packages/maven/io.arrow-kt:arrow-optics-ksp-plugin/1.2.0/compatibility-slim/1.1.5)](https://docs.renovatebot.com/merge-confidence/) | [![confidence](https://badges.renovateapi.com/packages/maven/io.arrow-kt:arrow-optics-ksp-plugin/1.2.0/confidence-slim/1.1.5)](https://docs.renovatebot.com/merge-confidence/) | | [io.arrow-kt:arrow-optics](https://togithub.com/arrow-kt/arrow) | `1.1.5` -> `1.2.0` | [![age](https://badges.renovateapi.com/packages/maven/io.arrow-kt:arrow-optics/1.2.0/age-slim)](https://docs.renovatebot.com/merge-confidence/) | [![adoption](https://badges.renovateapi.com/packages/maven/io.arrow-kt:arrow-optics/1.2.0/adoption-slim)](https://docs.renovatebot.com/merge-confidence/) | [![passing](https://badges.renovateapi.com/packages/maven/io.arrow-kt:arrow-optics/1.2.0/compatibility-slim/1.1.5)](https://docs.renovatebot.com/merge-confidence/) | [![confidence](https://badges.renovateapi.com/packages/maven/io.arrow-kt:arrow-optics/1.2.0/confidence-slim/1.1.5)](https://docs.renovatebot.com/merge-confidence/) | | [io.arrow-kt:arrow-core](https://togithub.com/arrow-kt/arrow) | `1.1.5` -> `1.2.0` | [![age](https://badges.renovateapi.com/packages/maven/io.arrow-kt:arrow-core/1.2.0/age-slim)](https://docs.renovatebot.com/merge-confidence/) | [![adoption](https://badges.renovateapi.com/packages/maven/io.arrow-kt:arrow-core/1.2.0/adoption-slim)](https://docs.renovatebot.com/merge-confidence/) | [![passing](https://badges.renovateapi.com/packages/maven/io.arrow-kt:arrow-core/1.2.0/compatibility-slim/1.1.5)](https://docs.renovatebot.com/merge-confidence/) | [![confidence](https://badges.renovateapi.com/packages/maven/io.arrow-kt:arrow-core/1.2.0/confidence-slim/1.1.5)](https://docs.renovatebot.com/merge-confidence/) | --- ### Release Notes <details> <summary>arrow-kt/arrow (io.arrow-kt:arrow-optics-ksp-plugin)</summary> ### [`v1.2.0`](https://togithub.com/arrow-kt/arrow/releases/tag/1.2.0) [Compare Source](https://togithub.com/arrow-kt/arrow/compare/1.1.5...1.2.0) #### What's Changed - Add CNAME file to Dokka output by [@franciscodr](https://togithub.com/franciscodr) in [arrow-kt/arrow#3029 - Remove legacy site code by [@nomisRev](https://togithub.com/nomisRev) in [arrow-kt/arrow#3030 - Update README by [@serras](https://togithub.com/serras) in [arrow-kt/arrow#3033 - Cancel previous PR action on new commit by [@nomisRev](https://togithub.com/nomisRev) in [arrow-kt/arrow#3032 - Remove legacy script files by [@franciscodr](https://togithub.com/franciscodr) in [arrow-kt/arrow#3034 - Ensure optics type with "data" modifier is a class by [@DeoTimeTheGithubUser](https://togithub.com/DeoTimeTheGithubUser) in [arrow-kt/arrow#3036 - Update all dependencies (major) by [@renovate](https://togithub.com/renovate) in [arrow-kt/arrow#3042 - Remove test dependency from Arrow Fx by [@nomisRev](https://togithub.com/nomisRev) in [arrow-kt/arrow#3046 - Bump Kotlin, KSP and coroutines version by [@nomisRev](https://togithub.com/nomisRev) in [arrow-kt/arrow#3047 - fix mapIndexed when collecting multiple times by [@hoc081098](https://togithub.com/hoc081098) in [arrow-kt/arrow#3056 - Update versions by [@nomisRev](https://togithub.com/nomisRev) in [arrow-kt/arrow#3058 - optics ksp plugin: fixed handling of variance ([#3057](https://togithub.com/arrow-kt/arrow/issues/3057)) by [@vladd-g](https://togithub.com/vladd-g) in [arrow-kt/arrow#3060 - Fixes recover inconsistency with raise DSL on types other than Either by [@yoxjames](https://togithub.com/yoxjames) in [arrow-kt/arrow#3052 - Change NonEmptySet type parameter name from T to A by [@franciscodr](https://togithub.com/franciscodr) in [arrow-kt/arrow#3062 - Add withError and (Eager)Effect.mapError by [@kyay10](https://togithub.com/kyay10) in [arrow-kt/arrow#3059 - Update versions of several libraries by [@serras](https://togithub.com/serras) in [arrow-kt/arrow#3017 - Add merge builder for raise by [@kyay10](https://togithub.com/kyay10) in [arrow-kt/arrow#3061 - Update all dependencies by [@renovate](https://togithub.com/renovate) in [arrow-kt/arrow#3065 - Update dependency gradle to v8.2 by [@renovate](https://togithub.com/renovate) in [arrow-kt/arrow#3070 - Fix warnings of single-subclass sealed classes by [@jooohn](https://togithub.com/jooohn) in [arrow-kt/arrow#3067 - KDoc for `Raise#raise`, `Raise#ensure` and `Raise#ensureNotNull` by [@ILIYANGERMANOV](https://togithub.com/ILIYANGERMANOV) in [arrow-kt/arrow#3038 - Fix text repetition in EffectScope's deprecation message by [@serras](https://togithub.com/serras) in [arrow-kt/arrow#3072 - Introduce `NonEmptyCollection` by [@serras](https://togithub.com/serras) in [arrow-kt/arrow#3068 - Set up Spotless by [@serras](https://togithub.com/serras) in [arrow-kt/arrow#3075 - Update all dependencies by [@renovate](https://togithub.com/renovate) in [arrow-kt/arrow#3079 - Add option to disable `inline` when using `@optics` by [@serras](https://togithub.com/serras) in [arrow-kt/arrow#3078 - Update JS versions in `yarn.lock` by [@serras](https://togithub.com/serras) in [arrow-kt/arrow#3084 - Enable Automatic Modules for JVM by [@serras](https://togithub.com/serras) in [arrow-kt/arrow#3071 - Serialization module by [@serras](https://togithub.com/serras) in [arrow-kt/arrow#3077 - Add missing docs for `Raise` operations by [@serras](https://togithub.com/serras) in [arrow-kt/arrow#3082 - Add mapOrAccumulate extension in RaiseAccumulate by [@nomisRev](https://togithub.com/nomisRev) in [arrow-kt/arrow#3086 - Additional tests for `copy` in Optics by [@serras](https://togithub.com/serras) in [arrow-kt/arrow#3089 - Apply Gradle Versioning in top project by [@serras](https://togithub.com/serras) in [arrow-kt/arrow#3092 - Add missing Versioning plug-in to `arrow-core-retrofit` by [@serras](https://togithub.com/serras) in [arrow-kt/arrow#3093 - Update `arrow-gradle-config` to 0.12-rc.4 by [@serras](https://togithub.com/serras) in [arrow-kt/arrow#3094 - MemoizedDeepRecursiveFunction by [@serras](https://togithub.com/serras) in [arrow-kt/arrow#3091 - Add reset and barrierAction to CyclicBarrier. by [@HSAR](https://togithub.com/HSAR) in [arrow-kt/arrow#3055 - \[HOTFIX] Fix main publish by [@nomisRev](https://togithub.com/nomisRev) in [arrow-kt/arrow#3095 #### New Contributors - [@DeoTimeTheGithubUser](https://togithub.com/DeoTimeTheGithubUser) made their first contribution in [arrow-kt/arrow#3036 - [@vladd-g](https://togithub.com/vladd-g) made their first contribution in [arrow-kt/arrow#3060 - [@yoxjames](https://togithub.com/yoxjames) made their first contribution in [arrow-kt/arrow#3052 - [@kyay10](https://togithub.com/kyay10) made their first contribution in [arrow-kt/arrow#3059 - [@jooohn](https://togithub.com/jooohn) made their first contribution in [arrow-kt/arrow#3067 - [@ILIYANGERMANOV](https://togithub.com/ILIYANGERMANOV) made their first contribution in [arrow-kt/arrow#3038 - [@HSAR](https://togithub.com/HSAR) made their first contribution in [arrow-kt/arrow#3055 **Full Changelog**: arrow-kt/arrow@1.2.0-RC...1.2.0 </details> --- ### Configuration 📅 **Schedule**: Branch creation - At any time (no schedule defined), Automerge - At any time (no schedule defined). 🚦 **Automerge**: Enabled. ♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the rebase/retry checkbox. 🔕 **Ignore**: Close this PR and you won't be reminded about these updates again. --- - [ ] If you want to rebase/retry this PR, check this box --- This PR has been generated by [Mend Renovate](https://www.mend.io/free-developer-tools/renovate/). View repository job log [here](https://developer.mend.io/github/sindrenm/android-project-template).

@nomisRev

* Added reset() and numberWaiting to CyclicBarrier. Both reset() and getNumberWaiting() are methods that exist on java.util.concurrent.CyclicBarrier and function in the same way. * Added barrierAction optional constructor variable to CyclicBarrier. This is also from java.util.concurrent.CyclicBarrier and executes when the barrier opens due to its capacity being reached. * Use sealed classes to model separate reset state in CyclicBarrier. * Implement reset on CyclicBarrier * Update API files * Retrigger CI after apiDump * Merge work @nomisRev and @HSAR * Apply suggestions from code review Co-authored-by: Alejandro Serrano <trupill@gmail.com> * Apply PR suggestions * Fix compilation issue * apiDump * Remove peekWaiting as discussed with @HSAR * apiDump * Fix tests --------- Co-authored-by: Simon Vergauwen <nomisRev@users.noreply.github.com> Co-authored-by: Alejandro Serrano <trupill@gmail.com>

Added reset() and numberWaiting to CyclicBarrier.

288a9f0

Both reset() and getNumberWaiting() are methods that exist on java.util.concurrent.CyclicBarrier and function in the same way.

HSAR commented May 17, 2023

View reviewed changes

arrow-libs/fx/arrow-fx-coroutines/src/commonMain/kotlin/arrow/fx/coroutines/CyclicBarrier.kt Outdated Show resolved Hide resolved

HSAR commented May 17, 2023

View reviewed changes

arrow-libs/fx/arrow-fx-coroutines/src/commonMain/kotlin/arrow/fx/coroutines/CyclicBarrier.kt Show resolved Hide resolved

HSAR changed the title ~~Added reset() and numberWaiting to CyclicBarrier.~~ Added reset() and barrierAction to CyclicBarrier. May 18, 2023

Added barrierAction optional constructor variable to CyclicBarrier.

07a0e92

This is also from java.util.concurrent.CyclicBarrier and executes when the barrier opens due to its capacity being reached.

HSAR force-pushed the cancellable-cyclic-barrier branch from 89457f5 to 07a0e92 Compare May 18, 2023 15:17

HSAR and others added 2 commits May 23, 2023 15:27

Use sealed classes to model separate reset state in CyclicBarrier.

a0f77f0

Merge branch 'main' into cancellable-cyclic-barrier

ed4d092

Merge branch 'main' into cancellable-cyclic-barrier

747fc34

Implement reset on CyclicBarrier

209ad17

nomisRev mentioned this pull request Jul 5, 2023

Implement reset on CyclicBarrier #3076

Closed

nomisRev added 3 commits July 5, 2023 10:22

Update API files

47f3f98

Retrigger CI after apiDump

3e73b0e

Merge branch 'sv-cyclicbarrier-reset' of github.com:arrow-kt/arrow in…

424dd5a

…to sv-cyclicbarrier-reset

nomisRev and others added 2 commits July 6, 2023 10:07

Merge branch 'main' into sv-cyclicbarrier-reset

af4368f

Merge branch 'sv-cyclicbarrier-reset' into cancellable-cyclic-barrier

ca5e75a

HSAR changed the title ~~Added reset() and barrierAction to CyclicBarrier.~~ Add reset to CyclicBarrier. Jul 6, 2023

Merge work @nomisRev and @HSAR

b3ac2d9

nomisRev requested review from franciscodr and serras July 6, 2023 13:45

HSAR commented Jul 6, 2023

View reviewed changes

arrow-libs/fx/arrow-fx-coroutines/src/commonMain/kotlin/arrow/fx/coroutines/CyclicBarrier.kt Outdated Show resolved Hide resolved

HSAR changed the title ~~Add reset to CyclicBarrier.~~ Add reset and barrierAction to CyclicBarrier. Jul 6, 2023

serras approved these changes Jul 6, 2023

View reviewed changes

nomisRev and others added 9 commits July 6, 2023 18:14

Apply suggestions from code review

5cd8b84

Co-authored-by: Alejandro Serrano <trupill@gmail.com>

Apply PR suggestions

3e95b5a

Merge branch 'main' into cancellable-cyclic-barrier

6d8ee91

Fix compilation issue

5a1badc

apiDump

b47f233

Merge branch 'main' into cancellable-cyclic-barrier

12ddf1b

Remove peekWaiting as discussed with @HSAR

0950d08

apiDump

a33fba0

Fix tests

e382d1e

nomisRev merged commit c8dfb0f into arrow-kt:main Jul 11, 2023
9 checks passed

HSAR deleted the cancellable-cyclic-barrier branch July 12, 2023 12:01

This was referenced Sep 8, 2023

Update all dependencies (optics-setup) arrow-kt/Arrow-JVM-Template#54

Merged

Update all dependencies (main) arrow-kt/Arrow-JVM-Template#53

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add reset and barrierAction to CyclicBarrier. #3055

Add reset and barrierAction to CyclicBarrier. #3055

HSAR commented May 17, 2023

HSAR commented May 17, 2023

nomisRev commented May 25, 2023

HSAR commented May 25, 2023

nomisRev commented Jul 3, 2023

HSAR commented Jul 3, 2023

nomisRev commented Jul 3, 2023

HSAR commented Jul 3, 2023

nomisRev commented Jul 4, 2023

HSAR commented Jul 4, 2023

nomisRev commented Jul 5, 2023

HSAR commented Jul 5, 2023 •

edited

Loading

HSAR commented Jul 5, 2023

nomisRev commented Jul 6, 2023

HSAR commented Jul 6, 2023

serras left a comment

nomisRev commented Jul 12, 2023

HSAR commented Jul 12, 2023

Add reset and barrierAction to CyclicBarrier. #3055

Add reset and barrierAction to CyclicBarrier. #3055

Conversation

HSAR commented May 17, 2023

HSAR commented May 17, 2023

nomisRev commented May 25, 2023

HSAR commented May 25, 2023

nomisRev commented Jul 3, 2023

HSAR commented Jul 3, 2023

nomisRev commented Jul 3, 2023

HSAR commented Jul 3, 2023

nomisRev commented Jul 4, 2023

HSAR commented Jul 4, 2023

nomisRev commented Jul 5, 2023

HSAR commented Jul 5, 2023 • edited Loading

HSAR commented Jul 5, 2023

nomisRev commented Jul 6, 2023

HSAR commented Jul 6, 2023

serras left a comment

Choose a reason for hiding this comment

nomisRev commented Jul 12, 2023

HSAR commented Jul 12, 2023

HSAR commented Jul 5, 2023 •

edited

Loading